Add basic assumptions to AGENTS.md#3552
Open
rambleraptor wants to merge 1 commit into
Open
Conversation
ebyhr
reviewed
Jun 24, 2026
| - One concern per PR. Keep unrelated formatting/import churn out of feature PRs. | ||
| - Keep the first version of a PR minimal; defer optimizations and edge cases to follow-ups. | ||
| - Commit messages explain the *what* and *why*, not line-by-line implementation. Be as succinct as possible. | ||
| - The Apache License header is required on every new source file (enforced by `./dev/check-license` and pre-commit). |
Member
There was a problem hiding this comment.
The license header isn't enforced by pre-commit as far as I confirmed.
Contributor
There was a problem hiding this comment.
This is done by ./dev/check-license
Contributor
There was a problem hiding this comment.
Ah, this is in there :)
Suggested change
| - The Apache License header is required on every new source file (enforced by `./dev/check-license` and pre-commit). | |
| - The Apache License header is required on every new source file (enforced by `./dev/check-license`). |
| - Formatting and linting are enforced by `ruff` via `prek` (pre-commit): line length **130**, double-quoted strings, isort with `pyiceberg`/`tests` as first-party. Run `make lint` — ruff autofixes most issues. | ||
| - Full type annotations are required (`mypy` runs in strict mode: `disallow_untyped_defs`, `no_implicit_optional`, `warn_unused_ignores`). Avoid Any types. | ||
| - Docstrings follow the project's pydocstyle config; one-line summaries on public functions. No personal pronouns in comments. | ||
| - Define typed exceptions in `pyiceberg/exceptions.py` and raise the most specific one; don't raise bare `Exception`. |
Member
There was a problem hiding this comment.
nit: ValueError is widely used in this project. We could rephrase it like this:
Use domain-specific exceptions from pyiceberg/exceptions.py for Iceberg-specific error conditions. For invalid arguments/values, ValueError is acceptable. Don't raise bare Exception.
Fokko
reviewed
Jun 30, 2026
| ## Testing | ||
|
|
||
| - Bias towards adding tests to existing files, rather than creating new files. | ||
| - Use existing test fixtures when possible. |
Contributor
There was a problem hiding this comment.
Should we add a line on our preference for integration testing over mocking?
Fokko
reviewed
Jun 30, 2026
|
|
||
| - Bias towards adding tests to existing files, rather than creating new files. | ||
| - Use existing test fixtures when possible. | ||
|
|
Contributor
There was a problem hiding this comment.
Maybe we should also say, avoid binaries if possible (for example, we use fastavro a lot to generate test-files).
Contributor
|
Thanks @rambleraptor for extending this, I like it a lot! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
Our AGENTS.md just talks about the security model. We get a non-trivial amount of AI activity on this repo and our AGENTS.md should reflect some basic assumptions we want for those PRs.
A lot of this was inspired by Iceberg AGENTS.md.
This is just meant to be an initial version with some thoughts that I've seen and some of the basic architecture (as seen in the Iceberg AGENTS.md)
Are these changes tested?
Just Markdown.
Are there any user-facing changes?
No.